feat(#10706): add move contact hierarchy - #11311
Conversation
Ports lineage-constraints: no cycles, only configured parent types, and no move that strands a place's primary contact. Two differences from cht-conf. It reads contact types through @medic/contact-types-utils rather than parsing the settings doc, since the server already has the parsed configuration. And it drops the check rejecting two sources from the same lineage, which exists because the CLI moves a list of contacts in one command; a move here has a single source. Violations are raised as BadRequestError so the caller gets the reason, and anything else, such as a database failure part way through validation, propagates instead of being reported as an invalid move.
Points a contact at a new parent lineage. Each operation carries the document id, the parent it is expected to still have, and the new minified lineage to write, so the handler computes nothing. An operation whose document has changed since the API gathered it is failed rather than clobbering the newer edit, matching set-contact. bulkDocs resolves even when individual writes fail, so every result is checked, and a failure does not stop the rest of the batch.
Gathers everything a move touches and queues it as a bulk operation. The subtree comes from contacts_by_depth, the reports the moved contacts authored come from the nouveau reports_by_freetext index queried on exact_match:"contact:<lowercased id>", and the surviving places whose primary contact moved come from contacts_by_primary_contact. Two actions are queued: set-parent for the contacts, and the existing set-contact for the reports and places, since in both of those the value being refreshed lives in a property called contact. Ids are gathered first and documents are then read a page at a time and turned straight into operations, so only one page is held at once. A district move can touch tens of thousands of documents and the request is the half of this that has to stay cheap. The nouveau results are paged with the bookmark rather than capped, so a prolific author is not silently truncated, and paging stops if the bookmark stops advancing. Ids are escaped before they are embedded in the quoted query phrase.
POST /api/v1/person/{id}/move and POST /api/v1/place/{id}/move, following the
per-type shape delete shipped with rather than a single /contact endpoint, so
a place cannot be moved through the person endpoint or the reverse.
Both share one handler and take a parent_id body property, where "root" moves
the contact to the top level, and an optional dry_run that returns the same
summary without queuing anything.
Gated on a new can_move_contact_hierarchy permission, which ships assigned to
no role. can_edit is held by almost every user and is a webapp permission, not
appropriate for guarding a server operation that rewrites a subtree.
Covers the permission gate, the wrong contact type, a missing parent_id, a circular move, a disallowed parent type, a move to the parent the contact already has, and a dry run that writes nothing. The end to end cases assert that the moved contact sits under the destination, that a descendant keeps its own parent with only the chain above it rewritten, and that the lineage cached on the reports the moved contacts authored follows while still naming the same author.
Splits the root-move check out of assertParentTypeIsAllowed and the paging loop out of getReportIdsByCreator, so neither exceeds the cognitive complexity threshold. Collapses the phrase escaping into a single replaceAll, and uses the dedicated chai matchers for undefined and null assertions.
…anagement-operations' into 10706-move
personFactory defaults every person to the patient_id 'test_woman_1', and a report records its subject's shortcode. contacts_by_depth emits that shortcode for the deleted subtree, so the delete tests matched the move fixtures' reports by subject and counted one report too many. Set an explicit shortcode on each move fixture person, as the person delete fixture already does.
jkuester
left a comment
There was a problem hiding this comment.
Good stuff here! I made it through a lot of the code and I want to go ahead and post the comments that I have so far. 👍
| * Queues an asynchronous bulk operation that moves the person and its whole subtree under a | ||
| * new parent, rewriting the parent lineage on every descendant and refreshing the cached | ||
| * lineage on their reports and on any place whose primary contact moved. Returns a summary | ||
| * of the changes and the bulk operation id to poll. |
There was a problem hiding this comment.
Minor, I think it is okay to not mention the primary contact rewrites. These should be relatively few and expected.
| * Queues an asynchronous bulk operation that moves the person and its whole subtree under a | |
| * new parent, rewriting the parent lineage on every descendant and refreshing the cached | |
| * lineage on their reports and on any place whose primary contact moved. Returns a summary | |
| * of the changes and the bulk operation id to poll. | |
| * Queues an asynchronous bulk operation that moves the person under a new parent. If the person is the | |
| * primary contact for any places, they will be updated with the person's new hierarchy. If the person is | |
| * associated with any user, all the reports written by that user will be updated with the person's new | |
| * hierarchy. Returns a summary of the changes and the bulk operation id to poll. |
| * $ref: '#/components/responses/NotFound' | ||
| */ | ||
| move: moveContactService.handleMove({ | ||
| get: (uuid) => getPerson(Qualifier.byUuid(uuid)), |
There was a problem hiding this comment.
Minor, can you add a 404 unit test like we have for delete?
| * Queues an asynchronous bulk operation that moves the place and its whole subtree under a | ||
| * new parent, rewriting the parent lineage on every descendant and refreshing the cached | ||
| * lineage on their reports and on any place whose primary contact moved. Returns a summary | ||
| * of the changes and the bulk operation id to poll. |
There was a problem hiding this comment.
I think it is okay to not mention the primary contact rewrites. These should be relatively few and expected.
| * Queues an asynchronous bulk operation that moves the place and its whole subtree under a | |
| * new parent, rewriting the parent lineage on every descendant and refreshing the cached | |
| * lineage on their reports and on any place whose primary contact moved. Returns a summary | |
| * of the changes and the bulk operation id to poll. | |
| * Queues an asynchronous bulk operation that moves the place and its whole subtree under a new parent. | |
| * If any moved persons are associated with a user, all the reports written by that user will be updated | |
| * with the person's new hierarchy. Returns a summary of the changes and the bulk operation id to poll. |
| * $ref: '#/components/responses/NotFound' | ||
| */ | ||
| move: moveContactService.handleMove({ | ||
| get: (uuid) => getPlace(Qualifier.byUuid(uuid)), |
There was a problem hiding this comment.
Same here. Can you add a 404 unit test like we have for delete?
|
|
||
| const { ACTIONS } = BULK_OPERATIONS; | ||
|
|
||
| const HIERARCHY_ROOT = 'root'; |
There was a problem hiding this comment.
I know this was just following the pattern from cht-conf. However, for well defined REST apis, I think we should avoid magic values like this. We should just be able to make the parent_id parameter optional. If it is not set, it means we should move the contact to the top-level.
|
|
||
| const { replaceContactLineage, replaceParentLineage } = require('./replace-lineage'); | ||
|
|
||
| const createLineageFromDoc = doc => { |
There was a problem hiding this comment.
Minor, but I think we could just use the minify.minifyLineage from shared-libs/lineage instead of having this function.
| let bookmark = null; | ||
|
|
||
| do { | ||
| const response = await request.post({ |
There was a problem hiding this comment.
Not sure this will work, but it is with a shot.... Can you try using the cht-datasource Report.v1.getUuids function here with Qualifier.byFreetext(terms.join(' OR '))? Looking through the code a bit, I cannot find any reason it should not work and it would be really nice to just have an AsyncGenerator here instead of having to manually page through the results...
| const getAncestorIdsToRefresh = async (contactIds) => { | ||
| const moved = new Set(contactIds); | ||
| const result = await db.medic.query('medic/contacts_by_primary_contact', { keys: contactIds }); | ||
| return [ ...new Set(result.rows.map(row => row.id).filter(id => !moved.has(id))) ]; |
There was a problem hiding this comment.
We are dropping any places here that are also being moved. However, if a place and its primary contact are both getting moved, we still want to update the contact value on the place to match the primary contact's new hierarchy. I think we maybe should not be filtering them out here. Or is that being handled somewhere else?
| for (let i = 0; i < ids.length; i += DOC_PAGE_SIZE) { | ||
| const page = ids.slice(i, i + DOC_PAGE_SIZE); |
There was a problem hiding this comment.
I liked the while/splice burn-down approach you used in getReportIdsByCreator. Can we use that here?
| const operations = []; | ||
| for (let i = 0; i < ids.length; i += DOC_PAGE_SIZE) { | ||
| const page = ids.slice(i, i + DOC_PAGE_SIZE); | ||
| const result = await db.medic.allDocs({ keys: page, include_docs: true }); |
There was a problem hiding this comment.
Wait, actually I think we might be able to get away with not fetching the docs here at all!
For the ancestorIds, the contacts_by_primary_contactview call already emits the current contact id as row.key. I think we just need to keep that value paired with the row.id....
For contactIds, I think we can get away with a view query (no include_docs: true): docs_by_id_lineage with keys: contactIds.map(id => [id, 1]) should give the id/current_parent_id values. Can do that query in batches of 10_000.
both current_parent_id and enough to rebuild every chain, because a descendant's chain up to the source lies entirely inside the subtree
We can use the same docs_by_id_lineage query for the reportIds too! Calling with the depth 1 will give us back the current_contact_id values.
Description
Adds the move half of the Advanced Contact Management API:
POST /api/v1/person/{id}/moveandPOST /api/v1/place/{id}/move, moving a contact and its whole subtree under a new parent.This is the second consumer of the bulk-operations framework that landed with delete in #11236, and it needs no changes to that framework. The design was reviewed on the Move tab of the technical design doc and follows what we settled there.
How it works. The request validates the move, gathers the documents it touches, and queues a bulk operation, returning a job id immediately. Sentinel applies the changes in batches, and the caller polls
GET /api/v1/bulk-operations/{id}.Two actions are queued, named for the field each writes:
set-parent(new)doc.parentset-contact(existing)doc.contactset-contactis reused unchanged because in both cases the value being refreshed lives in a property calledcontact: on a report it is the author reference, on a place it is the primary contact.Every operation is self-contained, carrying the document id, the value it expects to find, and the new value to write, so the handlers compute nothing. An operation whose document changed since it was gathered fails rather than clobbering the newer edit, and every
bulkDocsresult is checked.Validation. Ported from cht-conf's
lineage-constraints: no cycles, only configured parent types, no move that strands a place's primary contact, the source's own primary contact must be a person, and a move to the parent the contact already has is rejected rather than writing identical values back. Violations return a400with the reason; anything else, such as a database failure during validation, propagates as a500rather than being reported as an invalid move.Finding the reports. A move changes where the author sits, so the affected reports are the ones the moved contacts wrote, not the ones about them. These come from the nouveau
reports_by_freetextindex queried onexact_match:"contact:<lowercased id>". Results are paged with the bookmark rather than capped, so a prolific author is not silently truncated.Scale. Ids are gathered first and documents are then read a page at a time and turned straight into operations, so only one page is held in memory at once. A district move can touch tens of thousands of documents and the request is the half of this that has to stay cheap.
Permission. A new
can_move_contact_hierarchy, shipping assigned to no role.can_editis held by almost every user and is a webapp permission, not appropriate for guarding a server operation that rewrites a subtree.Endpoint shape. Per-type rather than a single
/contact/{uuid}endpoint, following what delete shipped with, so a place cannot be moved through the person endpoint or the reverse.One thing worth flagging for review: a move changes who can replicate the subtree. Moving a clinic between districts means the receiving district's offline users begin syncing those contacts and their reports, and the previous district's stop. That is what a move means, but it is an access change as much as a data change, which is part of why the permission is separate and admin-assigned.
The first commit lands the lineage manipulation helpers, which were written earlier but never merged while we focused on delete.
#10706
Code review checklist
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.